issue-245: fix CommonJS interoperability in tsc-multi and shim files #258
Conversation
|
|
I wasn't quite able to get this deployed and I'm not certain this works. If would like I will close |
There was a problem hiding this comment.
Thank you so much for sending this. As mentioned at one of the comments, this should be good to go, but we need more tests and add automated tests under https://github.com/openai/openai-agents-js/tree/main/integration-tests; I will work on it later this week. If you're willing to help us complete these too, it'd be appreciated.
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "targets": [ | |||
| { "extname": ".js", "module": "es2022", "moduleResolution": "node" }, | |||
| { "extname": ".js", "module": "commonjs", "moduleResolution": "node" }, | |||
There was a problem hiding this comment.
Thanks for changing this. This change looks reasonable, but we need to verify these before merging the change:
- Do more manual testing
- Add automated tests to ensure this works as expect under https://github.com/openai/openai-agents-js/tree/main/integration-tests
I will spend time on this later this week soon.
|
@seratch sure, feel to work on and close this pull request whenever. Unfortunately. I'm not sure if I will be able to work on in a timely fashion. This is no longer blocking me(was able to work around albeit took time) so no rush. You have my thanks |
Co-authored-by: Kazuhiro Sera <[email protected]>
|
hey @seratch! quick note up front: thanks to @CharlieGreenman for the import.meta shim idea, it’s folded in here. on the Lambda side: the node 22 runtime still ships without automatic module-syntax detection. i do not believe there is a public commitment from aws to change that e.g. see aws-lambda-base-images#233. so here we’re still hitting two gaps that need addressing. (1) build artifacts: we need both the ESM and CJS outputs, properly wired via the exports field, so environments like lambda and jest can safely require() the SDK without breaking compatibility for existing ESM consumers. (2) there’s runtime safety. I think the sdk's shims must avoid unguarded access to Given everything that needs to land here, I think a fresh PR is the cleanest path forward. I’m putting one together to handle both the dual-build setup and the shim guard. (and would include integration tests that cover import, require, and execution inside the relevant Lamda runtime). I'll have it wrapped up tmrw for review. |
|
Thanks for the inputs. I believe the direction should be good to go, but I just don't have time to do thorough testing right now. Any inputs / suggestions / contributions for this would be greatly appreciated. |
|
Just a quick update that this work has been superseded by PR #333. which rolls in a full dual esm + cjs build, adds import.meta shim guards, and includes integration tests for Lambda and cjs runtimes It covers the same cjs interoperability issues raised here (#245, #213) but also covers other serverless environments, so it should (after feedback) fully address the use cases we discussed in this thread. |
|
Thanks for your contribution! I've moved your commit to #346; if you have any comments, please feel free to write in! |
Solves:
I am building my own personal npm package. Will confirm here when works as expected (for AWS Lambda)